Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto highlight selected #397

Closed

Conversation

tris203
Copy link

@tris203 tris203 commented Dec 9, 2023

#396 but to the right branch
Adds request in #384

@tris203
Copy link
Author

tris203 commented Dec 9, 2023

@willothy thoughts?

Copy link

@willothy willothy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far, but you're not setting the highlight, just the cursor :)

lua/harpoon/ui.lua Show resolved Hide resolved
lua/harpoon/utils.lua Show resolved Hide resolved
Copy link
Author

@tris203 tris203 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review

lua/harpoon/utils.lua Show resolved Hide resolved
lua/harpoon/ui.lua Show resolved Hide resolved
@tris203
Copy link
Author

tris203 commented Dec 10, 2023

I assume, given the direction that this will need adjusting to pull the logic out of UI

I would guess the best way is to make toggle_quick_menu() take a second optional param of CurrentItem which is used for the highlighting/focus logic

Then a function passed back from the API get_current() which contains the logic for determining the name of the current item

@ThePrimeagen
Copy link
Owner

so i am going to be removing all highlighting from Harpoon and i am going to start emitting out events with win/buf id, and perhaps some extra state such that you can add whatever highlights you wish

i don't want harpoon having too much logic. So i am "likely" not going to accept any PRs that add logic but instead adding hooks

@willothy
Copy link

willothy commented Dec 10, 2023

This does raise an important point (even if the highlighting is removed from this PR). Not sure how I didn't think of this before, but what if the list items aren't paths? I think that if there is going to be a way to determine the current item, it needs to be a method on the List Config so that different lists can modify the default behavior. It also needs to be optional because for some lists (commands, for example) there is no "current" item, so it would not make sense to show one.

@ThePrimeagen
Copy link
Owner

@willothy there is equals and display

From the UI we call display, else we call equals

@willothy
Copy link

willothy commented Dec 11, 2023

Yes, sorry I should've been more clear. I mean that we're always checking nvim_buf_get_name(0) at the moment so even if we do use equals and display, we're still only able to check against lists of buffers because the current thing in equals(list_item, current) will always be the name of the current buffer.

For example, say we want to make a harpoon list of packages in a project. In this example the items are still paths, yet there's no way to determine the current item (or it would be tricky, at least - you'd have to split and match the current buf path). This would get more complex if the items aren't paths at all, though I can't really think of examples of that other than commands.

I think there should be a current method on the list config that defaults to vim.api.nvim_buf_get_name(0) but could be overridden to use things such as vim.uv.cwd(), etc. as needed.

@ThePrimeagen
Copy link
Owner

yes, these things can be tricky

to me its an information problem. perhaps we could provide "from_buffer" or some option to let you know whence you came

@willothy
Copy link

to me its an information problem. perhaps we could provide "from_buffer" or some option to let you know whence you came

Yes, that's perfect. A more generic way to check "is this buffer related to this item" than checking the name.

@pockata
Copy link
Contributor

pockata commented Dec 11, 2023

I was just about to rewrite in Lua that SOB that I PR'd in #199 to drop the vim.cmd and string.format calls, but I noticed this PR :)

I just wanted to point out that with vim.api.nvim_buf_add_highlight when you dd the highlighted entry and paste it again it will lose the highlighting. That's why the better approach is to use vim.fn.matchadd (perhaps vim.fn.matchaddpos too, but I haven't used it) which preserves the highlighting after an edit (technically reapplies it automatically when a string matches the pattern).

HarpoonCurrentFile isn't widely used because you have to dig into the source code to find it. That's bad marketing on my behalf. If it was present in the README, for example, I believe this would have been a fan favorite feature. An incredibly sensible default of highlight link HarpoonCurrentFile String is an amazing way to spot the current item in the list blazingly fast, especially if you're a bit dyslexic. Having the highlighting in a hook which you grab from a "Recipes" wiki page is also a good way to go, but having it as a built-in default will provide a much better experience for new and experienced users alike.

I suggest you add the highlight command in your config, test drive it for a couple of days and then remove it to see if you miss it :)

@ThePrimeagen
Copy link
Owner

Coming back to this i think i want to go a different direction.

i would like to think about how to make this a extension not something inside of harpoon. If you have an idea for an event to emit where you could hook in and do this work yourself, please do. I would have no problems with the harpoon-colors plugin doing something for me. I just don't want to maintain that :)

@ThePrimeagen
Copy link
Owner

if you look at harpoon/init.lua and search for this line self._extensions:emit(Extensions.event_names.LIST_CREATED, list) you can do the same thing for hooking into the buf / window created.

@tris203 tris203 closed this May 1, 2024
@ThePrimeagen
Copy link
Owner

I am planning on doing another harpoon season coming up.

I just feel pulled in every direction, it's very difficult, but I'm very excited

I will make sure this gets somewhat resolved

@tris203
Copy link
Author

tris203 commented May 2, 2024

@ThePrimeagen I can reopen the PR if you think it is still useful?

It was your other direction comment made me think it was no longer relevant

@pockata
Copy link
Contributor

pockata commented May 2, 2024

This is already possible with the events system. I've created a small plugin that adds this functionality. The actual code is so small that you can copy it to your config and save yourself a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants